crypto: don't reference |freelist_max_len| in OpenSSL 1.1.0. #10859

agl · 2017-01-17T20:21:38Z

The |freelist_max_len| (and the freelist itself) has been removed in
OpenSSL 1.1.0. Thus this change will be necessary at some point but,
for now, it makes it a little easier to build with 1.1.0 without
breaking anything for previous versions of OpenSSL.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

crypto

sam-github · 2017-01-17T20:44:32Z

src/node_crypto.cc

@@ -1147,10 +1147,14 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {


 void SecureContext::SetFreeListLength(const FunctionCallbackInfo<Value>& args) {
+#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)


I'm OK with this, but another approach would be to simply not define .setFreeListLength() for ossl versions that don't support it. Its not a public API, we don't have to keep it, and then the code in _tls_common.js would just not call the API if it doesn't exist.

Happy to do this if people prefer, although you would have to tell me what the recommended pattern in Javascript is for testing whether a function exists or not.

Lets get one more person to weigh in. My pitch is that it is the first step towards just removing the function, which we will want to do, why keep a function around that does nothing? Its very confusing! Also, it makes feature testing very easy.

But its a waste of your time to refactor like I suggest unless @nodejs/crypto or someone else agrees. Lets listen.

You would basically take the ifdef you have around the fn internals now and move them to https://github.com/agl/node/blob/c6404a264e5587dedd0a495f1c91e6d90edcc7ac/src/node_crypto.cc#L297 so the js binding does not get defined. If there are no other uses of SetFreeListLength, you'd have to ifdef out its prototype and def'n as well to avoid compiler whinging.

pattern for testing whether function exists before use is just

if(c.context.setFreeListLength) c.context.setFreeListLength(0);

I think the current approach is fine. Sam's suggestion would make the diff quite a bit larger.

I agree, current approach is better. We can clean up all at once later.

sam-github · 2017-01-17T20:47:46Z

Commit message shouldn't have a ., and should be <= 50 chars. You can remove the test checkbox from the PR description, and the docs, it needs neither.

agl · 2017-01-17T21:54:23Z

Commit updated so that the first line is <= 50 chars.

mscdex · 2017-01-17T22:54:30Z

Minor nit: I think the pipes can be dropped in the commit message.

The freelist_max_len member of SSL* (and the freelist itself) has been removed in OpenSSL 1.1.0. Thus this change will be necessary at some point but, for now, it makes it a little easier to build with 1.1.0 without breaking anything for previous versions of OpenSSL.

bnoordhuis · 2017-01-18T07:04:08Z

src/node_crypto.cc

@@ -1147,10 +1147,14 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {


 void SecureContext::SetFreeListLength(const FunctionCallbackInfo<Value>& args) {
+#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)


I think the current approach is fine. Sam's suggestion would make the diff quite a bit larger.

indutny

LGTM

indutny · 2017-01-18T14:11:39Z

src/node_crypto.cc

@@ -1147,10 +1147,14 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {


 void SecureContext::SetFreeListLength(const FunctionCallbackInfo<Value>& args) {
+#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)


I agree, current approach is better. We can clean up all at once later.

sam-github · 2017-01-18T17:02:52Z

Sam's suggestion would make the diff quite a bit larger.

Isn't bigger better? 💪

OK, I'm good with approach as-is, LGTM (still)

shigeki · 2017-01-19T02:59:49Z

How about disabling freelist entirely? It gives more simplicity and compatibility between 1.0.2 and 1.1.0.

diff --git a/deps/openssl/openssl.gypi b/deps/openssl/openssl.gypi
index 871cec0..e260cb5 100644
--- a/deps/openssl/openssl.gypi
+++ b/deps/openssl/openssl.gypi
@@ -1268,6 +1268,9 @@
       # the real driver but that poses a security liability when an attacker
       # is able to create a malicious DLL in one of the default search paths.
       'OPENSSL_NO_HW',
+
+      # Disable freelist for it has no longer benefits
+      'OPENSSL_NO_BUF_FREELISTS'
     ],
     'openssl_default_defines_win': [
       'MK1MF_BUILD',
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index f843667..af2d729 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -1150,7 +1150,7 @@ void SecureContext::SetFreeListLength(const FunctionCallbackInfo
   SecureContext* wrap;
   ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());

-  wrap->ctx_->freelist_max_len = args[0]->Int32Value();
+  // Do nothing. To be deprecated in the future.
 }

bnoordhuis · 2017-01-19T12:22:19Z

@shigeki I had the same thought but that would reintroduce the memory consumption issue in builds that link against a shared openssl. If we feel that's acceptable, then by all means go for it.

sam-github · 2017-01-19T19:28:43Z

I read through #1522 and I don't see the connection to "shared ssl", @bnoordhuis, what is different about shared SSL vs ours?

bnoordhuis · 2017-01-19T19:38:42Z

@sam-github The system openssl won't be compiled with -DOPENSSL_NO_BUF_FREELISTS so if we don't zero freelist_max_len, it's going to use (lots) more memory.

sam-github · 2017-01-19T19:50:36Z

I don't think we should break user's of system SSL, it seems unhelpful to linux distros, with not much gain to us. Once we move to new OpenSSL versions, this code seems like it will naturally go away, so long term, we won't have to maintain it.

shigeki · 2017-01-23T05:17:36Z

I missed thinking of shared openssl. It means we cannot change and fix ssl behaviors by using build defines or opensslconf.h unfortunately.
This PR has enough approvals so I will land this now.

shigeki · 2017-01-23T05:22:01Z

CI is running on https://ci.nodejs.org/job/node-test-pull-request/5991/

shigeki · 2017-01-23T06:18:51Z

CI is green. Landed in a57e2f2. Thanks.

The freelist_max_len member of SSL* (and the freelist itself) has been removed in OpenSSL 1.1.0. Thus this change will be necessary at some point but, for now, it makes it a little easier to build with 1.1.0 without breaking anything for previous versions of OpenSSL. PR-URL: nodejs#10859 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

MylesBorins · 2017-03-08T02:18:50Z

I've gone ahead and landed this on v4.x and v6.x staging... the test suites are passing.

if for any reason this should be reverted please @ me

The freelist_max_len member of SSL* (and the freelist itself) has been removed in OpenSSL 1.1.0. Thus this change will be necessary at some point but, for now, it makes it a little easier to build with 1.1.0 without breaking anything for previous versions of OpenSSL. PR-URL: #10859 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github · 2017-03-08T18:10:27Z

I think backporting is right, @nodejs/crypto any of you disagree?

The freelist_max_len member of SSL* (and the freelist itself) has been removed in OpenSSL 1.1.0. Thus this change will be necessary at some point but, for now, it makes it a little easier to build with 1.1.0 without breaking anything for previous versions of OpenSSL. PR-URL: #10859 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

shigeki · 2017-03-09T14:35:43Z

I agree this. It gives a code consistency.

The freelist_max_len member of SSL* (and the freelist itself) has been removed in OpenSSL 1.1.0. Thus this change will be necessary at some point but, for now, it makes it a little easier to build with 1.1.0 without breaking anything for previous versions of OpenSSL. PR-URL: nodejs/node#10859 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

This function was introduced in 2684c90 as an internal helper function. The C++ implementation became a no-op in a57e2f2 when building against OpenSSL 1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all supported OpenSSL versions in 970ce14. Finally, eb20447 removed the only call site of setFreeListLength (which was already a no-op at that point). Refs: nodejs#1529 Refs: nodejs#10859 Refs: nodejs#19794 Refs: nodejs#38116

This function was introduced in 2684c90 as an internal helper function. The C++ implementation became a no-op in a57e2f2 when building against OpenSSL 1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all supported OpenSSL versions in 970ce14. Finally, eb20447 removed the only call site of setFreeListLength (which was already a no-op at that point). Refs: #1529 Refs: #10859 Refs: #19794 Refs: #38116 PR-URL: #44300 Reviewed-By: Feng Yu <F3n67u@outlook.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This function was introduced in 2684c90 as an internal helper function. The C++ implementation became a no-op in a57e2f2 when building against OpenSSL 1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all supported OpenSSL versions in 970ce14. Finally, eb20447 removed the only call site of setFreeListLength (which was already a no-op at that point). Refs: nodejs#1529 Refs: nodejs#10859 Refs: nodejs#19794 Refs: nodejs#38116 PR-URL: nodejs#44300 Reviewed-By: Feng Yu <F3n67u@outlook.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This function was introduced in 2684c90 as an internal helper function. The C++ implementation became a no-op in a57e2f2 when building against OpenSSL 1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all supported OpenSSL versions in 970ce14. Finally, eb20447 removed the only call site of setFreeListLength (which was already a no-op at that point). Refs: #1529 Refs: #10859 Refs: #19794 Refs: #38116 PR-URL: #44300 Reviewed-By: Feng Yu <F3n67u@outlook.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This function was introduced in 2684c90 as an internal helper function. The C++ implementation became a no-op in a57e2f2 when building against OpenSSL 1.1.0 (instead of OpenSSL 1.0.2), and eventually became a no-op in all supported OpenSSL versions in 970ce14. Finally, eb20447 removed the only call site of setFreeListLength (which was already a no-op at that point). Refs: nodejs/node#1529 Refs: nodejs/node#10859 Refs: nodejs/node#19794 Refs: nodejs/node#38116 PR-URL: nodejs/node#44300 Reviewed-By: Feng Yu <F3n67u@outlook.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem. lts-watch-v6.x labels Jan 17, 2017

sam-github approved these changes Jan 17, 2017

View reviewed changes

agl force-pushed the freelist branch from 8081a33 to 91a5c9d Compare January 17, 2017 21:54

nodejs-github-bot added lts-watch-v6.x labels Jan 17, 2017

agl force-pushed the freelist branch from 91a5c9d to c6404a2 Compare January 17, 2017 23:14

nodejs-github-bot added lts-watch-v6.x labels Jan 17, 2017

bnoordhuis approved these changes Jan 18, 2017

View reviewed changes

indutny approved these changes Jan 18, 2017

View reviewed changes

jasnell approved these changes Jan 18, 2017

View reviewed changes

shigeki approved these changes Jan 23, 2017

View reviewed changes

shigeki closed this Jan 23, 2017

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 8, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

rvagg mentioned this pull request Nov 10, 2017

Support both OpenSSL 1.1.0 and 1.0.2 #16130

Closed

3 tasks

tniessen mentioned this pull request Aug 20, 2022

tls: remove SecureContext setFreeListLength #44300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: don't reference |freelist_max_len| in OpenSSL 1.1.0. #10859

crypto: don't reference |freelist_max_len| in OpenSSL 1.1.0. #10859

agl commented Jan 17, 2017 •

edited

Loading

sam-github Jan 17, 2017

agl Jan 17, 2017

sam-github Jan 18, 2017 •

edited

Loading

bnoordhuis Jan 18, 2017

indutny Jan 18, 2017

sam-github commented Jan 17, 2017

agl commented Jan 17, 2017

mscdex commented Jan 17, 2017

bnoordhuis Jan 18, 2017

indutny left a comment

indutny Jan 18, 2017

sam-github commented Jan 18, 2017

shigeki commented Jan 19, 2017

bnoordhuis commented Jan 19, 2017

sam-github commented Jan 19, 2017

bnoordhuis commented Jan 19, 2017

sam-github commented Jan 19, 2017

shigeki commented Jan 23, 2017

shigeki commented Jan 23, 2017

shigeki commented Jan 23, 2017

MylesBorins commented Mar 8, 2017

sam-github commented Mar 8, 2017

shigeki commented Mar 9, 2017

		@@ -1147,10 +1147,14 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo<Value>& args) {


		void SecureContext::SetFreeListLength(const FunctionCallbackInfo<Value>& args) {
		#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)

crypto: don't reference |freelist_max_len| in OpenSSL 1.1.0. #10859

crypto: don't reference |freelist_max_len| in OpenSSL 1.1.0. #10859

Conversation

agl commented Jan 17, 2017 • edited Loading

Checklist

Affected core subsystem(s)

sam-github Jan 17, 2017

Choose a reason for hiding this comment

agl Jan 17, 2017

Choose a reason for hiding this comment

sam-github Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

bnoordhuis Jan 18, 2017

Choose a reason for hiding this comment

indutny Jan 18, 2017

Choose a reason for hiding this comment

sam-github commented Jan 17, 2017

agl commented Jan 17, 2017

mscdex commented Jan 17, 2017

bnoordhuis Jan 18, 2017

Choose a reason for hiding this comment

indutny left a comment

Choose a reason for hiding this comment

indutny Jan 18, 2017

Choose a reason for hiding this comment

sam-github commented Jan 18, 2017

shigeki commented Jan 19, 2017

bnoordhuis commented Jan 19, 2017

sam-github commented Jan 19, 2017

bnoordhuis commented Jan 19, 2017

sam-github commented Jan 19, 2017

shigeki commented Jan 23, 2017

shigeki commented Jan 23, 2017

shigeki commented Jan 23, 2017

MylesBorins commented Mar 8, 2017

sam-github commented Mar 8, 2017

shigeki commented Mar 9, 2017

agl commented Jan 17, 2017 •

edited

Loading

sam-github Jan 18, 2017 •

edited

Loading